Transformations0.6#80
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
It would be good to test this with @jo-mueller's test samples listed at https://forum.image.sc/t/ngff-weekly-dev-update-thread/110810/98. |
|
German-BioImaging/incubator#72 Sample images to test against |
I will mirror these in the image.sc dev thread |
davehorsfall
left a comment
There was a problem hiding this comment.
This is good. Left some comments for discussion.
| coordinateSystems?: CoordinateSystem[]; | ||
| } | ||
|
|
||
| //Once parsing and transforming is set up, it should be possible to significantly simplify these types |
There was a problem hiding this comment.
Please also point here from the Issue that is setup to track full implementation of the transformations from the ZOD schema parsing.
| ); | ||
| console.log("Applying scene transformations to image: ", config.source); | ||
| const sceneModelMatrix = coordinateTransformationsToMatrix(transformations, [ | ||
| { type: "channel", name: "c" }, |
There was a problem hiding this comment.
This appears to be passing a hard-coded 4-element [c, z, y, x] axes array to coordinateTransformationsToMatrix regardless of the scene's actual coordinate systems. Is that intended, and long term?
coordinateTransformationsToMatrix also seems to throw and error if the shape of the axes and the transformations don't match.
There was a problem hiding this comment.
Changed to dynamically select the first coordinate system axes in the scene metadata.
Left a TODO for future work to allow the user to change between coordinate systems.
Linked TODO to sub-issue in existing issue #69
| mat = coordinateTransformationsToMatrix(transform.transformations, axes, mat); | ||
| } | ||
| if (transform.type === "affine") { | ||
| console.log("Affine transformation detected"); |
There was a problem hiding this comment.
Perhaps also add an Issue to track support for this as it develops, and as test datasets start to be generated/shared.
There was a problem hiding this comment.
Uncommented this as it actually works - it doesn't have any impact on the test image because the affine transformation defined is the identity. I think this was commented out while debugging at some point.
| //No type information for SceneSchema | ||
| scene: Ome.Scene, | ||
| ): Promise<SourceData[]> { | ||
| console.log("Loading scene: ", config.source); |
There was a problem hiding this comment.
Currently there are lots of informational logs printed to the console (even in production?)
It would be good to start wrapping this into the app level logger (and error handler) we've been discussing, so it is easier to control what is printed, and where.
There was a problem hiding this comment.
Agreed to add these once logging PR has been merged #95
|
@davehorsfall addressed comments and workflows passing. |
|
https://ome.github.io/ome-ngff-validator/?source=https://livingobjects.ebi.ac.uk/idr/zarr/test-data/v0.6.dev4/idr0050/4995115_output_to_ms.zarr is a more up-to-date scene example, although slightly more complex than the v0.6.dev3 one above. The dev3 one is failing validation because it's using latest schemas which have been updated for dev4. |
davehorsfall
left a comment
There was a problem hiding this comment.
LGTM.
The spec is evolving, and new examples are emerging. We will track these examples in new issues, to avoid this PR getting blocked by the quickly changing landscape.
Description
Implements scene specification and 0.6 multiscales coordinateSystems.
Adds test images including scene. URL can be found at fixtures/scene.yaml e.g.
https://uk1s3.embassy.ebi.ac.uk/idr/zarr/test-data/v0.6.dev3/idr0050/4995115_scene.zarr
NOTE
Due to image metadata mis-specification, the image WILL NOT align correctly while conforming to the specification rules. However, if the additional 'rotated' transformations are included the image DOES align correctly (i.e. the rotation transformation logic is correctly implemented). This is because the scene-level transformations specified here
https://will-moore.github.io/ome-ngff-validator/?source=https://uk1s3.embassy.ebi.ac.uk/idr/zarr/test-data/v0.6.dev3/idr0050/4995115_scene.zarr
for each image have input coordinate system 'physical', meaning we should only apply the coordinate transformations with coordinate system 'physical' from these images.